tools: refactor tools/deps_updater
#46488
Conversation
|
Review requested:
|
nodejs-github-bot
added
meta
.github/workflows/tools.yml
Outdated
| @@ -123,27 +111,13 @@ jobs: | |||
| label: dependencies | |||
| run: | | |||
| NEW_VERSION=$(gh api repos/libuv/libuv/releases/latest -q '.tag_name|ltrimstr("v")') | |||
There was a problem hiding this comment.
I was unsure how to port this line into the update script. I'll take a deeper look.
If someone has an idea, feel free to share ^
There was a problem hiding this comment.
I missed that earlier, but FYI you could do something along the lines of
NEW_VERSION="$("$NODE" --input-type=module <<'EOF'
const res = await fetch('https://api.github.com/repos/libuv/libuv/releases/latest');
if (!res.ok) throw new Error(`FetchError: ${res.status} ${res.statusText}`, { cause: res });
const { tag_name } = await res.json();
console.log(tag_name.replace('v', ''));
EOF)"There was a problem hiding this comment.
Do you want to try to fit that in? Or we can land this as is, and take this to a follow up PR, you've been dragging this one for long enough :)
There was a problem hiding this comment.
I definitely prefer to try it ^^ It will abstract the logic a bit more into the script which is the first motivation of this PR.
There was a problem hiding this comment.
done 🎉 @aduh95 let me know WDYT ^^
We have something homogenous now. GH scripts and updater scripts share same responsabilites.
There was a problem hiding this comment.
Linter is not happy btw. I didn't see any parse error on my machine. I'll take a deeper look.
There was a problem hiding this comment.
Please consider copy/pasting the text rather than taking screenshots, as your color settings may be hard to decipher for the rest of us (e.g. folks with visual disabilities), and is also more data to download, which is never nice for those on limited data plans 🙂
I don’t think this is ideal, wouldn’t adding a line return before closing the parentheses work, as the linter suggests? Meaning replacing EOF) with EOF\n). FYI you can run the linter locally using tools/lint-sh.mjs path/to/script.sh.
There was a problem hiding this comment.
Sorry, I didn't explain, but the screenshots were to show the syntax highlight that was totally broken with the previous version.
I finally add the put the EOF in the line below and it worked!
tony-go
changed the title
tools/deps_updatertools/deps_updater
tools/dep_updaters/update-libuv.sh
Outdated
| CURRENT_SUFFIX_VERSION=$(grep "#define UV_VERSION_SUFFIX" "$VERSION_H" | sed -n "s/^.*SUFFIX \"\(.*\)\"/\1/p") | ||
| SUFFIX_STRING=$([ -z "$CURRENT_SUFFIX_VERSION" ] && echo "" || echo "-$CURRENT_SUFFIX_VERSION") | ||
| CURRENT_VERSION="$CURRENT_MAJOR_VERSION.$CURRENT_MINOR_VERSION.$CURRENT_PATCH_VERSION$SUFFIX_STRING" |
There was a problem hiding this comment.
I realize it's existing code that's been moved around but it might be simpler to ignore UV_VERSION_SUFFIX and instead check that UV_VERSION_IS_RELEASE is 1. The former is always an empty string when the latter is 1.
There was a problem hiding this comment.
Hey @bnoordhuis 👋🏼
Could you please let me know if it looks okay on your side?
tony-go
force-pushed
the
refactor-tools-depsupdater
branch
4 times, most recently
from
8fbc9af
to
040671b
Compare
|
Note to me: refactor ada script too. |
tony-go
force-pushed
the
refactor-tools-depsupdater
branch
from
e14bb28
to
f3ef6b1
Compare
aduh95
added
the
author ready
aduh95
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/46488 ✔ Done loading data for nodejs/node/pull/46488 ----------------------------------- PR info ------------------------------------ Title tools: refactor `tools/deps_updater` (#46488) Author Tony Gorez (@tony-go) Branch tony-go:refactor-tools-depsupdater -> nodejs:main Labels meta, tools, author ready Commits 19 - tools: refactor dep_updaters - tools: use $NPM and $NODE - tools: make postject and eslint uniform - fix: lib message - tools: use quotes for handling spaces in $VERSION_H - tools: use $NODE and $NPM in postject script - fix: delete useless statement - fix: make libuv script posix compliant - tools: use UV_IS_RELEASE - fix: add NEW_VERSION into GITHUB_ENV - fix: put $GITHUB_ENV logic out off the script - fix: use proper script fro postject - fix: refactor - fix: simplify pre-scripts - fix: check console output for libuv and simtduf scripts - fix: simplify script - tools: abstract new version fetch into scripts - fix: scripts syntax - fix: add fallback to avoir job failure Committers 1 - Tony Gorez PR-URL: https://github.com/nodejs/node/pull/46488 Reviewed-By: Antoine du Hamel Reviewed-By: Darshan Sen ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46488 Reviewed-By: Antoine du Hamel Reviewed-By: Darshan Sen -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 03 Feb 2023 15:23:28 GMT ✔ Approvals: 2 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46488#pullrequestreview-1321370764 ✔ - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/46488#pullrequestreview-1321391778 ⚠ GitHub cannot link the author of 'tools: refactor dep_updaters' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'tools: use $NPM and $NODE' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'tools: make postject and eslint uniform' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: lib message' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'tools: use quotes for handling spaces in $VERSION_H' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'tools: use $NODE and $NPM in postject script' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: delete useless statement' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: make libuv script posix compliant' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: add NEW_VERSION into GITHUB_ENV' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: put $GITHUB_ENV logic out off the script' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: use proper script fro postject' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ⚠ GitHub cannot link the author of 'fix: simplify pre-scripts' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 46488 From https://github.com/nodejs/node * branch refs/pull/46488/merge -> FETCH_HEAD ✔ Fetched commits as 024e648d905d..489e7d3cf4e1 -------------------------------------------------------------------------------- Auto-merging .github/workflows/tools.yml [main 21f392cfa0] tools: refactor dep_updaters Author: Tony Gorez Date: Fri Feb 3 16:07:07 2023 +0100 5 files changed, 53 insertions(+), 39 deletions(-) [main f7cb667646] tools: use $NPM and $NODE Author: Tony Gorez Date: Fri Feb 3 17:12:20 2023 +0100 1 file changed, 9 insertions(+), 7 deletions(-) [main 29e9649342] tools: make postject and eslint uniform Author: Tony Gorez Date: Fri Feb 3 17:15:31 2023 +0100 2 files changed, 6 insertions(+), 7 deletions(-) [main 41c49b660e] fix: lib message Author: Tony Gorez Date: Fri Feb 3 20:09:50 2023 +0100 1 file changed, 1 insertion(+), 1 deletion(-) [main f8a4bafe38] tools: use quotes for handling spaces in $VERSION_H Author: Tony Gorez Date: Fri Feb 3 20:13:00 2023 +0100 1 file changed, 4 insertions(+), 4 deletions(-) [main 24240ffecf] tools: use $NODE and $NPM in postject script Author: Tony Gorez Date: Fri Feb 3 20:15:40 2023 +0100 1 file changed, 2 insertions(+), 2 deletions(-) [main 03efe1d861] fix: delete useless statement Author: Tony Gorez Date: Fri Feb 3 20:27:18 2023 +0100 1 file changed, 1 deletion(-) [main 49b2df363a] fix: make libuv script posix compliant Author: Tony Gorez Date: Fri Feb 3 20:29:59 2023 +0100 1 file changed, 1 insertion(+), 1 deletion(-) [main fc5a46207e] tools: use UV_IS_RELEASE Author: Tony Gorez Date: Sun Feb 5 17:15:31 2023 +0100 1 file changed, 2 insertions(+), 1 deletion(-) [main 2a9be4ae20] fix: add NEW_VERSION into GITHUB_ENV Author: Tony Gorez Date: Wed Feb 8 17:40:36 2023 +0100 4 files changed, 8 insertions(+) Auto-merging .github/workflows/tools.yml [main c40cce9506] fix: put $GITHUB_ENV logic out off the script Author: Tony Gorez Date: Wed Feb 8 18:10:34 2023 +0100 5 files changed, 21 insertions(+), 13 deletions(-) Auto-merging .github/workflows/tools.yml [main 75fc67692c] fix: use proper script fro postject Author: Tony Gorez Date: Thu Feb 9 10:31:55 2023 +0100 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging .github/workflows/tools.yml [main 41ef44d78e] fix: refactor Author: Tony Gorez Date: Sat Feb 18 15:19:22 2023 +0100 3 files changed, 16 insertions(+), 12 deletions(-) Auto-merging .github/workflows/tools.yml [main e7b77266e0] fix: simplify pre-scripts Author: Tony Gorez Date: Mon Feb 20 17:09:32 2023 +0100 1 file changed, 2 insertions(+), 4 deletions(-) Auto-merging .github/workflows/tools.yml [main ad4ee98c52] fix: check console output for libuv and simtduf scripts Author: Tony Gorez Date: Thu Feb 23 11:30:18 2023 +0100 1 file changed, 8 insertions(+), 4 deletions(-) Auto-merging .github/workflows/tools.yml [main 638f4cc200] fix: simplify script Author: Tony Gorez Date: Thu Feb 23 12:15:58 2023 +0100 1 file changed, 2 insertions(+), 2 deletions(-) Auto-merging .github/workflows/tools.yml [main 302acbd92e] tools: abstract new version fetch into scripts Author: Tony Gorez Date: Fri Feb 24 12:21:00 2023 +0100 3 files changed, 30 insertions(+), 24 deletions(-) [main b1a88a8432] fix: scripts syntax Author: Tony Gorez Date: Fri Feb 24 14:12:32 2023 +0100 2 files changed, 6 insertions(+), 4 deletions(-) Auto-merging .github/workflows/tools.yml [main 4abd332311] fix: add fallback to avoir job failure Author: Tony Gorez Date: Thu Mar 2 08:57:29 2023 +0100 1 file changed, 4 insertions(+), 4 deletions(-) ✔ Patches applied There are 19 commits in the PR. Attempting autorebase. Rebasing (2/38)https://github.com/nodejs/node/actions/runs/4313711979 |
aduh95
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in ac6ceff |
|
Thanks for your work here. If you (or someone else) want to continue the work here to migrate the other updater scripts, that'd be awesome. |
Yeah I'll take care of that ^^ Thanks you for you amazing support and guidance during this journey ^^ |
PR-URL: #46488 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
PR-URL: #46488 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
PR-URL: #46488 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
Ref: #46157 (comment)
This PR aims to port logic from GitHub action into proper scripts.
Signed-off-by: Tony Gorez [email protected]
How to test
tony-go:refactor-tools-depsupdater./tools/dep_updaters/update-libuv.sh 1.44.2./tools/dep_updaters/update-simdutf.sh 3.1.0./tools/dep_updaters/update-postject.sh./tools/dep_updaters/update-eslint.sh